Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Yaml Validator into Persistence Part 2 #700

Merged
merged 24 commits into from
Jul 23, 2024

Conversation

abaskk-msft
Copy link
Contributor

@abaskk-msft abaskk-msft commented Jul 15, 2024

PR Update

  • Move Yaml validator out of Persistence again with the existing refactoring updates. As Persistence uses .net8 , while pac supports several other .net verisons.
  • The intention is to still package YamlValidator into a separate NuGet package from Persistence (due to compatbility issues with PAC CLI). The build process of signing this binary and adding it to the build pipeline will be done in a separate pr.

Problem

  • At it's current state, the YamlValidator can't be properly consumed by pac CLI to perform validation
  • This re-write fixes that with a factory class to build validators (which can be extended if need be in the future)

Notes

  • The factory for the Validator is barebones, however some extensions that can be made using the factory include:
  • Error handling:
    • The Validator will gracefully handle any errors that involve edge cases when validating yaml strings (empty, not yaml etc) as well as any issues with the schema in the nuget package
  • File I/O error will always be handled by pac cli
image

Validation

  • Unit tests
  • Added unit test for factory class as well

Schema Loading

  • Instead of using file I/O and building to a local directory, schemas are built into the assembly itself , subschemas and schemas are then loaded into memory, like bolt.module.connector does

@abaskk-msft abaskk-msft requested review from a team as code owners July 15, 2024 21:54
Copy link
Contributor

@joem-msft joem-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great progress. Please address the comments that I've raised.

src/Directory.Build.props Outdated Show resolved Hide resolved
src/Directory.Build.props Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/SchemaLoader.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/Validator.cs Show resolved Hide resolved
.version/PipelineAssemblyInfo.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/SchemaLoader.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/SchemaLoader.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/ValidationProcessor.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/ValidationRequest.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/Validator.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/ValidatorFactory.cs Outdated Show resolved Hide resolved
src/Persistence/YamlValidator/IValidatorFactory.cs Outdated Show resolved Hide resolved
@abaskk-msft abaskk-msft requested a review from joem-msft July 19, 2024 17:37
@abaskk-msft abaskk-msft requested a review from tehcrashxor July 22, 2024 17:26
@abaskk-msft abaskk-msft dismissed joem-msft’s stale review July 23, 2024 20:09

implemented suggested changes

@abaskk-msft abaskk-msft merged commit b2a2aef into master Jul 23, 2024
4 checks passed
@abaskk-msft abaskk-msft deleted the users/t-abaskar/persistence-yaml-validator-pac branch July 23, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants